repr: canonicalize float encodings in row packing - #37482
Open
ggevay wants to merge 3 commits into
Open
Conversation
ggevay
force-pushed
the
gabor/sql-452-literalconstraints-optimization-causes-wrong-results
branch
from
July 7, 2026 10:59
0f95e35 to
a76d983
Compare
antiguru
self-requested a review
July 7, 2026 11:08
Contributor
Author
|
(I'm still investigating some test fails before bringing it out of draft.) Edit: fixed |
MINIMUM_CONSOLIDATED_VERSION gated assumptions about codec-order consolidation: parts written before it could not be assumed to be consolidated or sorted according to the current definition. Its last consumers were removed in f02f202 ("Remove codec-order consolidation") when consolidation moved entirely to structured (Arrow) ordering with per-run RunOrder metadata, leaving the constant unreferenced. Also update the comment on the SourceData serialization stability test, which still instructed encoding changes to bump the constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SQL float equality is semantic: -0.0 = 0.0 and NaN = NaN are true, and Datum equality agrees (via OrderedFloat). But packed rows are compared as raw bytes (Row equality, arrangement keys, index lookups), and -0.0 vs +0.0 and different NaN payloads have distinct bit patterns. Equal values therefore landed under distinct arrangement keys, giving wrong results for index lookups (LiteralConstraints), joins, GROUP BY, DISTINCT, DISTINCT ON, and UNION on float columns. Fix this at the single choke point that writes float bytes: row packing now rewrites each float to the canonical representative of its equality class, -0.0 to +0.0 and every NaN to the quiet positive NaN, mirroring the existing Numeric canonicalization. All ingress paths (persist columnar decode, ProtoRow decode, source decoders, pgwire) go through the packer, so pre-existing persisted data is normalized on read. Text formatting renders -0.0 as "0" as well, so text output does not depend on whether a value crossed a packing boundary (a view and a materialized view of the same query render identically). Negative zero is thereby unobservable in Materialize. This deviates from PostgreSQL, which prints "-0". Also correct the warning comment on the "=" operator table, which claimed BinaryFunc::Eq is byte equality. It is Datum equality, and packing must canonicalize any type whose Datum equality is coarser than bit equality. Fixes SQL-452. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ggevay
force-pushed
the
gabor/sql-452-literalconstraints-optimization-causes-wrong-results
branch
2 times, most recently
from
July 7, 2026 13:40
9833ce1 to
5c999cf
Compare
Make the remaining sign-of-zero-sensitive scalar functions treat -0.0 as +0.0, so that results do not depend on whether a value crossed a packing boundary (e.g. a view vs a materialized view of the same query): cot(-0.0) now returns +Infinity (PostgreSQL returns -Infinity here), power no longer errors for a -0.0 base with a fractional exponent or a -0.0 exponent with a zero base, and ln/log10 (float and numeric) report the zero error rather than the negative error for -0.0. The ln/log10/power changes also match PostgreSQL, which uses zero-first checks and strict comparisons, and they let NaN inputs propagate to NaN results regardless of the NaN's sign bit. Regenerate the SourceData serialization stability snapshot: old encodings containing non-canonical floats now decode to canonical rows, so re-encoding them produces different bytes. Update the stale comment that pointed at MINIMUM_CONSOLIDATED_VERSION, which has not been consulted since codec-order consolidation was removed. Add platform checks covering the cross-version story for tables, Postgres sources, and Kafka upsert sources: rows written by an old version with raw float bits must cancel against retractions written by a new version, and must land in the same DISTINCT/GROUP BY/index groups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ggevay
force-pushed
the
gabor/sql-452-literalconstraints-optimization-causes-wrong-results
branch
from
July 7, 2026 14:59
5c999cf to
08a90d5
Compare
ggevay
marked this pull request as ready for review
July 7, 2026 15:17
Contributor
Author
|
This is ready for review! (Nightly's failing Orchestratord tests are unrelated.) |
martykulma
reviewed
Jul 14, 2026
martykulma
left a comment
Contributor
There was a problem hiding this comment.
It seems like there might be some impact to sinks:
- kafka upsert with float in the key
- kafka sink w/debezium envelope
kafka compaction will leave stranded messages (floats in the key are already problematic, so I'm not sure how much worse this is just because floats are problematic). Anything that cares about definite outputs is going to see some impact.
Upsert iceberg sink isn't affected, it disallows floats in keys. For non-key cases (e.g. float in the value), there's customer visibility, but presumably no impact.
How are we communicating to users?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of #37489. The first (tiny) commit is from there.
Fixes SQL-452.
SQL float equality is semantic: -0.0 = 0.0 and NaN = NaN are true, and Datum equality agrees (via OrderedFloat). But packed rows are compared as raw bytes (Row equality, arrangement keys, index lookups), and -0.0 vs +0.0 and different NaN payloads have distinct bit patterns. Equal values therefore landed under distinct arrangement keys, giving wrong results for index lookups (LiteralConstraints), joins, GROUP BY, DISTINCT, DISTINCT ON, and UNION on float columns.
Fix this at the single choke point that writes float bytes: row packing now rewrites each float to the canonical representative of its equality class, -0.0 to +0.0 and every NaN to the quiet positive NaN, mirroring the existing Numeric canonicalization. All ingress paths (persist columnar decode, ProtoRow decode, source decoders, pgwire) go through the packer, so pre-existing persisted data is normalized on read.
A visible consequence is that floats never render as "-0" anymore. Row packing canonicalizes stored values, and text formatting renders -0.0 as "0" as well, so text output does not depend on whether a value crossed a packing boundary (a view and a materialized view of the same query render identically). Negative zero is thereby unobservable in Materialize. This deviates from PostgreSQL, which prints "-0".
Also correct the warning comment on the "=" operator table, which claimed BinaryFunc::Eq is byte equality. It is Datum equality, and packing must canonicalize any type whose Datum equality is coarser than bit equality.
Sign-of-zero-sensitive functions
A few scalar functions could observe the sign of a float zero, which would make their results depend on whether the input crossed a packing boundary (e.g. a view vs a materialized view of the same query). They now treat -0.0 exactly like +0.0:
cot(-0.0)returns+Infinity. This deviates from PostgreSQL, which returns-Infinity.power(-0.0, 0.5)returns0andpower(0, -0.0)returns1instead of erroring. This matches PostgreSQL, which uses strict comparisons rather than the sign bit. NaN inputs now propagate to NaN results regardless of the NaN's sign bit, also matching PostgreSQL.ln/log/log10(float and numeric) report the "not defined for zero" error for -0.0 rather than the "not defined for negative numbers" error, matching PostgreSQL's zero-first check order.SourceData encoding stability
The
source_proto_serialization_stabilitygolden file is regenerated: old encodings containing non-canonical floats now decode to canonical rows, so re-encoding them produces different bytes. Consequence for existing shards: a record whose old and new encodings differ never consolidates away inside persist (an old-version addition and a new-version retraction of the same logical row both stay in the shard). Readers stay correct because they consolidate rows after decoding, where the two encodings are identical. The stability test's comment pointed atMINIMUM_CONSOLIDATED_VERSIONas the gate for such changes, but that constant has been unused since codec-order consolidation was removed in f02f202. It is deleted in #37489, which this PR is stacked on (the first commit here is that PR's commit until it merges). Persist reviewers, please confirm this reasoning.Compatibility notes
-0.0emitted before this change encodes different bytes than the same key emitted after it, so a post-upgrade update or deletion of such a row will not match the pre-upgrade key in the topic, leaving a phantom entry behind. Floats in upsert sink keys are a degenerate setup, but worth calling out.-0.0row written before this change and its retraction written after it no longer byte-cancel during compaction. They still cancel at read time (every read decodes through the packer, which normalizes), so results are correct. The cost is a bounded amount of never-consolidated data in affected shards.test/sqllogictest/cockroach/float.sltstays disabled: its correctness expectations now hold, but its display expectations assume CockroachDB's key/value separation (stored-0preserved on output), which this design intentionally does not provide.Testing
New platform checks (
FloatCanonicalizationTable,FloatCanonicalizationPgCdc,FloatCanonicalizationUpsert) exercise the cross-version story: rows written by an older version with raw float bits must cancel against retractions written by a newer version, and must land in the same DISTINCT/GROUP BY/index groups. This covers tables, Postgres sources, and Kafka upsert sources (including a -0.0 vs +0.0 upsert key).New unit test
test_float_packing_canonicalizesinmz-reprcovering signed zeros, negative/payload/signaling NaNs, for bothfloat4andfloat8.New regression tests in
test/sqllogictest/transform/literal_constraints.slt: indexed lookups across-0.0/+0.0/NaN in both directions, a no-index control, and an EXPLAIN showing= '0' OR = '-0'dedupes to a single lookup key.New tests in
test/sqllogictest/float.sltfor the arrangement family: join, GROUP BY, DISTINCT, DISTINCT ON, UNION over mixed zeros and NaN payloads, the-0.0rendering, and a view vs materialized view of the same query rendering identically. The existing'-0'::float::textexpectations change from-0to0.Nightly run: https://buildkite.com/materialize/nightly/builds/17039